feat(quinn): Refactor polling & sending to take &mut self
#67
Conversation
matheus23
changed the title
poll_send-based AsyncUdpSocket abstraction&mut self
matheus23
force-pushed
the
matheus23/mut-self
branch
from
871e5c6 to
54043ca
Compare
There was a problem hiding this comment.
Overall seems fine, but needs a lot of docs updates.
One main concern is that I'd like to be relatively sure that this isn't creating a performance impact on upstream Quinn. I believe @matheus23 has been working on this already.
The other is what upstream mentioned: this is a breaking change. For us this would be 0.14. I'd also prefer if we managed to move this upstream. I understand they also have concerns about the semvers-incompatible change. We should already start a PR and have it go through review to a "basically accepted" state, at which point I guess it'd be waiting for when they make the next semver-incompatible release. I believe there are already a few such issues queued up, so this will probably happen at some point.
| /// If this returns [`io::ErrorKind::WouldBlock`], [`UdpPoller::poll_writable`] must be called | ||
| /// to register the calling task to be woken when a send should be attempted again. | ||
| fn try_send(&self, transmit: &Transmit) -> io::Result<()>; | ||
| fn create_sender(&self) -> Pin<Box<dyn UdpSender>>; |
There was a problem hiding this comment.
The doc comments here needs updating.
| @@ -91,71 +79,119 @@ pub trait AsyncUdpSocket: Send + Sync + Debug + 'static { | |||
| /// | |||
| /// Any number of `UdpPoller`s may exist for a single [`AsyncUdpSocket`]. Each `UdpPoller` is | |||
| /// responsible for notifying at most one task when that socket becomes writable. | |||
| pub trait UdpPoller: Send + Sync + Debug + 'static { | |||
| pub trait UdpSender: Send + Sync + Debug + 'static { | |||
| /// Helper adapting a function `MakeFut` that constructs a single-use future `Fut` into a | ||
| /// [`UdpPoller`] that may be reused indefinitely | ||
| struct UdpPollHelper<MakeFut, Fut> { | ||
| pub struct UdpSenderHelper<Socket, MakeFut, Fut> { |
There was a problem hiding this comment.
doc comments. I'd like to understand what fut is expected to do here.
There was a problem hiding this comment.
Another TODO here: Do we want this to be pub? I initially turned this from private to pub because I thought it'd be useful to us in net-tools land in the netwatch socket impl, but turns out we can't quite use the helper there.
I generally find the helper... helpful? Usually anytime I implement AsyncUdpSocket, but it wasn't exposed in quinn before.
| // obtain an `&mut Fut` after storing it in `self.fut` when `self` is already behind `Pin`, | ||
| // and if we didn't store it then we wouldn't be able to keep it alive between | ||
| // `poll_writable` calls. | ||
| let result = ready!(this.fut.as_mut().as_pin_mut().unwrap().poll(cx)); |
There was a problem hiding this comment.
I'm still not sure what fut is expected to be doing when reading this code.
There was a problem hiding this comment.
Agreed (this is inherited from the previous quinn code). I considered renaming it to WritableFut.
Polling this future tells us if the socket is writable, and wakes us up if so.
|
|
||
| /// TODO(matheus23): Docs | ||
| /// Last ditch/best effort of sending a transmit. | ||
| /// Used by the endpoint for resets / close frames when dropped, etc. |
There was a problem hiding this comment.
seems like more than last-ditch since poll_send itself is implemented as a function of this? Anyway, a plea for better docs :)
There was a problem hiding this comment.
The fact that it's used like that in the implementation of the tokio and async-io sockets is an implementation detail.
The documentation in the trait has to be more general.
| } | ||
| } | ||
|
|
||
| pub trait UdpSenderHelperSocket: Send + Sync + 'static { |
There was a problem hiding this comment.
Docs on why this exists would be helpful. Seems like the reason is that it allows calling try_send and max_transmit_segments on the socket by the helper. Maybe the trait can also be called something like that? Like TrySendTransmit or something?
No description provided.